-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8374169: too many unsafe intrinsics (simplify unsafe intrinsic processing) #28940
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
👋 Welcome back jrose! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
Status: No testing beyond looking for smoke when I run javac (in the changed VM). I have not gone looking for inlining failures. I know they are endemic in C2. I think the interpreter support (native methods) is stable, as are JDK code adjustments. The vmIntrinsics.hpp declarations define a bunch of “polymorphic intrinsic wrappers” (F_PW). C2 support is not started yet. It should be relatively simple, as with C1. The C1 support is nearly stable. Smoke test with javac passes. I added some C1 canonicalization logic for back-to-back conversions. |
|
Overall goal is to thin out the current Dark Forest of intrinsics, which has become overgrown. |
ExE-Boss
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the final modifier should be kept on the Unsafe methods to ease the burden on reviewing the git diff.
|
|
||
| /** Special-access version of {@link #getInt(Object, long)} */ | ||
| @ForceInline | ||
| public int getIntMO(byte memoryOrder, Object o, long offset) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be better to keep these parametrised memory order implementation methods private without changing the Java API by changing @IntrinsicCandidate to @ForceInline on all the [get/put/compareAnd[Set/Exchange]]<Type>[/Opaque/Acquire/Release/Volatile] methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, exporting the primitive type polymorphism is too much, at least for now. (I know about the one public method that is still private.) The BT values can go private as well.
As you can see, I am experimenting with polymorphism of memory ordering, although I think we need to retain the Volatile versions of everything, which I find annoying. But the Plain Acquire Release and Opaque versions are just too niche-y; the power user that wants them needs to just use the new MO constants.
Regarding the final modifier, 1. it has no force since the class as a whole is already final, 2. it has been used inconsistently in the API to date, and 3. there is lots of review friction in this big changeset, which may require breaking it up into a queue of changesets. We’ll probably do that breakup when the time for integration comes closer.
The OP constants will stay public, to reduce the number of names while still providing full functionality. Also, RISC-V has min and max reduction ops (signed and unsigned) and that suggests to me that we are not done adding OP modes, for the get-and-operate API points.
|
@rose00 |
|
One idea behind this new polymorphism of MO and BT and OP: The Unsafe API is not useful as an important arbitration point for what hardware operations are surfaced upward through the JDK. The JITs have a much more fundamental role as arbitration points. Unsafe “gets out of the way” by providing all reasonable combinations of MO and BT and OP, and then the JITs can make the call (following the evolution of hardware) what to expose. In the previous configuration, get-and-bitop (bitop = and, xor, etc.) are provided by our major platforms, and yet they are inaccessible because Unsafe provides a choke-point that blocks them from the rest of the system. BTW, get-and-max might be a good way to handle monotonically versioned values. And 128-bit atomics are here today, but not visible yet. Valhalla comes first to open up the possibilities for a 128-bit hyper-long, and then we can consider surfacing the 128-bit hardware primitives. Polymorphism makes it easy to climb the hill of functionality without slipping into a combinatorial swamp. |
|
BTW, I am not asking for anybody to press the Review button, yet. This is a draft review. If you have a helpful comment, please do make it. (Thanks, ExE-Boss.) But pressing the Review button, at this point, doesn’t help me with this task. Also, pressing the review button with no comments is the same as adding a silly comment, identifying the button-presser as a noise source. |
|
| @IntrinsicCandidate | ||
| public native long getLong(Object o, long offset); | ||
| @ForceInline | ||
| public long getLong(Object o, long offset) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the advantage of having these convenience methods enough to merit the 2x number of methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which convenience methods would you remove? (What is the 2x?) Removing getLong(ref,long) is a big ask. I am deliberately making the getPrimitiveBits guys (the real new primitives) be private to Unsafe.
I have tried a few different experiments removing convenience methods. The system usually fought back, and in many cases I restored them. (E.g., I tried removing getLong(long) and had to back out; there seems to be too much JDK code using the no-reference versions, sadly.)
I would like to remove Unsafe API points, but the main thing I am attempting here is to simplify the interface between Unsafe and the underlying VM intrinsics scheme.
| * which is the logical combination of both {@code MO_ACQUIRE} | ||
| * and {@code MO_RELEASE}, in one operation. | ||
| */ | ||
| public static final byte MO_PLAIN = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would we risk running out of bits if we only use an 8-bit representation, considering potential new memory ordering types like "stable"? Would the performance not be the same if we use an int rather than a byte?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t think we will run out of bits, even with bytes. We can widen to shorts or ints, but I think that is unlikely, even for the MO_XXX constants. (Famous last words; the internal accessDecorators are a beefy 64 bits. But they are far more than we want here.)
There are small benefits to using subwords (bytes or maybe shorts); the main one is that it is impossible to mix up the order of arguments, since you can only pass one of the MO or BT or OP constants to a byte. Byte is almost like boolean — nothing promotes to it, so if the static types line up, you are probably OK.
We could use ints, sure. But there are really not that many configuration choices here.
Because the interfaces are private to Unsafe, we can widen from byte if we need to do so.
(I had a wild idea, also, of packing the prefix bytes (intrinsic config) into a 64-bit long, along with the vmI::ID, and using that as a cookie for intrinsic lookup. But that seems to be unnecessary.)
| @ForceInline | ||
| public long getLongMO(byte memoryOrder, Object o, long offset) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would also be possible to use an access scheme similar to the one in MemorySegment, where there is only one get method, but with different overrides for different layouts. Is this something worth exploring?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think this is a possibility. My first cut here is to refactor the lower surface of Unsafe. (With some opportunistic simplifications to its upper surface.) But moving forward, the "primitive-bits" protocol could be shared with a few other clients. It’s even sharper-edged than getLong; you can crash real fast by passing a bad size-value to getPrimitiveBitsMO. But it might allow the MemorySegment to get simpler, in its lower surface that faces Unsafe. Same point for VarHandles.
Doing this will require making getPrimitiveBitsMO public in Unsafe. Some folks have already expressed discomfort with such a move, but I think we could come around to it. Maybe by making a pact that only a few classes will actually use that "maximally unsafe" API point.
|
Dan, thanks for reading the javadoc, especially the essays about memory order. I trust you found it reasonably lucid and informative. I’m slightly surprised you didn’t mention any errors. (Should have snuck in an intentional error…) I’d welcome edits to the content from you or any other memory model practitioner. |
first cut: passes smoke tests in interpreter and C1 (aarch64)
https://bugs.openjdk.org/browse/JDK-8374169
too many intrinsics
Fold getIntAcquire and every other getFooBar into getPrimitiveBitsMO(mo, bt, base, offset).
Do similar moves for put and the various CAS operations.
Progress
Integration blocker
Warning
8374169: too many unsafe intrinsics (simplify unsafe intrinsic processing)Issue
Reviewers without OpenJDK IDs
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/28940/head:pull/28940$ git checkout pull/28940Update a local copy of the PR:
$ git checkout pull/28940$ git pull https://git.openjdk.org/jdk.git pull/28940/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 28940View PR using the GUI difftool:
$ git pr show -t 28940Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/28940.diff